fix: countryCode property in Input type phone number by vicky-primathon · Pull Request #7684 · appsmithorg/appsmith

vicky-primathon · 2021-09-21T10:39:21Z

Description

Fix countryCode property in Input type phone number

Fixes #7475

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

Test coverage results 🧪

🟢 Total coverage has increased

// Code coverage diff between base branch:release and head branch: fix/selected-phone-number-country-code

Status	File	% Stmts	% Branch	% Funcs	% Lines
🟢	total	54.81 (0.03)	36.68 (0.07)	33.54 (0.02)	55.34 (0.02)
🟢	app/client/src/utils/DSLMigrations.ts	73.78 (0.16)	65.09 (-0.05)	70.21 (0)	73.59 (0.16)
🟢	app/client/src/utils/autocomplete/TernServer.ts	52.26 (0.22)	41.25 (0.83)	34.48 (0)	56.22 (0.26)
✨ 🆕	app/client/src/utils/migrations/InputWidget.ts	80	80	75	79.17
🟢	app/client/src/widgets/InputWidget/widget/index.tsx	64.52 (2.76)	43.75 (2.08)	68.18 (0)	67.05 (3.13)

riodeuno · 2021-09-23T06:50:25Z

@somangshu I don't think this completely solve the issue here. When I try to bind the Input1.text I don't get the country code. There is also no other property which has the country code. Not sure, if this was a part of the original issue. Please review the deploy preview.

somangshu · 2021-09-23T07:06:41Z

@riodeuno I can see that for the type phone number the {{input.countryCode}} works and returns the short code, This however should return the int code and not the short code.

@vicky-primathon I see that this does not function well in case of the currency as well. We need to fix the issue.

somangshu

Change as mentioned above

vicky-primathon · 2021-09-23T10:27:16Z

@riodeuno I can see that for the type phone number the {{input.countryCode}} works and returns the short code, This however should return the int code and not the short code.

@vicky-primathon I see that this does not function well in case of the currency as well. We need to fix the issue.

@somangshu in case of currency, selected country code can be accessed as {{input.currencyCountryCode}}
Should we expose single api i.e {{input.countryCode}} for both phone number and currency selected country code.

somangshu · 2021-09-23T10:34:18Z

@somangshu in case of currency, selected country code can be accessed as {{input.currencyCountryCode}}
Should we expose single api i.e {{input.countryCode}} for both phone number and currency selected country code.

@vicky-primathon when I check:

currencyCountryCode does not return anything. It should return the ISO code
currencyCountryCode should be changed to currencyCode.
The countryCode should return the phone extension ex: +91

Let me know if you have any doubts

somangshu

Deploy preview looks good to me, All changes as requested are updated.

@riodeuno can you please verify the code changes

riodeuno · 2021-09-23T15:24:44Z

app/client/src/widgets/InputWidget/component/ISDCodeDropdown.tsx

      label: `${item.name} (${item.dial_code})`,
-      value: item.code,
-      id: item.dial_code,
+      value: item.dial_code,


@vicky-primathon Will these changes effect existing users? How do we verify this in the UI?

Will have to write migrations to update default selected code value from country code to dial_code

@riodeuno added migrations for this

@vivekverma2312 We will need to verify the migrations during QA. The approach would be open a bound input widget in this deploy preview, which was created in release.

app/client/src/utils/migrations/InputWidget.ts

riodeuno · 2021-10-04T11:26:27Z

Note for QA: We need a regression test for these types of input widgets.
cc @YogeshJayaseelan @vivekverma2312

vivekverma2312 · 2021-10-11T10:35:45Z

@vicky-primathon

If input field is empty then {{Input1.countryCode}} - {{Input1.text}} is showing undefined in text box.

During migration {{Input2.currencyCountryCode}}{{Input2.text}} is not migrating to {{Input2.currencyCode}} {{Input2.text}}. Due to this changing currency in Input widget do not change the currency in text widget.

vicky-primathon · 2021-10-26T08:19:53Z

/ok-to-test sha=51c2f21

vicky-primathon · 2021-10-26T08:21:00Z

https://github.com/appsmithorg/appsmith/actions/runs/1384732996

vicky-primathon · 2021-10-26T11:08:14Z

/ok-to-test sha=51c2f21

vicky-primathon · 2021-10-26T11:11:44Z

https://github.com/appsmithorg/appsmith/actions/runs/1385337655

vicky-primathon · 2021-10-26T12:10:04Z

/ok-to-test sha=51c2f21

vicky-primathon · 2021-10-26T12:10:28Z

https://github.com/appsmithorg/appsmith/actions/runs/1385545269

riodeuno · 2021-10-26T12:21:59Z

app/client/src/utils/migrations/InputWidget.ts

+    for (const key in child) {
+      if (
+        typeof child[key] === "string" &&
+        child[key].includes(".currencyCountryCode")


I can see that we're migrating bindings in the DSLs. However, I was wondering about the case where the bindings are in the actions? This seems like in those scenarios the applications might fail.
So, I was wondering if this was possible to be bound so far? From the looks of it, this feature wasn't functional so far, and we might be able to get away with only migrating DSLs.
@somangshu @vivekverma2312 @vicky-primathon

That would be right, Since the feature has just been released we could expect minimum usage of this. However if a user complains what would be our response? Is there a way we can check migrations for the action as well? Since we just need to replace all occurrence, logically this is straightforward

Migrating actions on the client will be tricky. We will need to implement a migration strategy for actions as well. Before we can get into this, we need to check with the backend to see if there are established mechanisms to deal with this. @sharat87

Meanwhile, @vicky-primathon can you find out when this commit was merged into release?

@somangshu #5256

riodeuno · 2021-10-26T12:23:38Z

One comment. The rest looks good.

vicky-primathon · 2021-10-28T10:12:45Z

/ok-to-test sha=75aad2b

vicky-primathon · 2021-10-28T11:00:00Z

https://github.com/appsmithorg/appsmith/actions/runs/1394118204

…ncyCode

…number dial code

… code to currency migrations"

somangshu · 2021-11-09T09:19:59Z

@vicky-primathon @riodeuno I disagree to CountryCode changing to DialCode, this is not the generic convention.

cc @Nikhil-Nandagopal

Also since this is blocked by the backend migration, should we close this or draft it until unblocked

riodeuno · 2021-11-09T11:36:59Z

@somangshu I don't have strong opinions on this. countryCode works.
@vicky-primathon Let's make this into a draft for now.

somangshu · 2021-11-09T12:30:05Z

@riodeuno I said that because the name used in the first release used countryCode and if we continue with it we will not need any migrations.

sbalaji1192 · 2021-11-12T10:19:38Z

@vicky-primathon Lets close this PR since we're gonna introduce a v2 of input widget. We will do these changes on the v2.

vicky-primathon added the UI Building Pod label Sep 21, 2021

vicky-primathon requested a review from akash-codemonk September 21, 2021 10:39

github-actions bot added the Bug Something isn't working label Sep 21, 2021

vicky-primathon requested review from riodeuno and removed request for akash-codemonk September 22, 2021 06:37

riodeuno requested a review from somangshu September 23, 2021 06:50

somangshu suggested changes Sep 23, 2021

View reviewed changes

vicky-primathon requested a review from somangshu September 23, 2021 11:28

somangshu previously approved these changes Sep 23, 2021

View reviewed changes

riodeuno reviewed Sep 23, 2021

View reviewed changes

vicky-primathon dismissed somangshu’s stale review via 4128bd1 September 29, 2021 08:04

vicky-primathon requested a review from riodeuno September 29, 2021 08:17

riodeuno requested changes Oct 4, 2021

View reviewed changes

app/client/src/utils/migrations/InputWidget.ts Outdated Show resolved Hide resolved

vicky-primathon requested a review from riodeuno October 4, 2021 09:02

riodeuno previously approved these changes Oct 4, 2021

View reviewed changes

vicky-primathon dismissed riodeuno’s stale review via 1316890 October 6, 2021 09:40

riodeuno previously approved these changes Oct 7, 2021

View reviewed changes

somangshu added the Widgets Product This label groups issues related to widgets label Oct 15, 2021

vicky-primathon dismissed riodeuno’s stale review via 5f32380 October 26, 2021 08:10

vicky-primathon force-pushed the fix/selected-phone-number-country-code branch from 5f32380 to 51c2f21 Compare October 26, 2021 08:18

vicky-primathon requested a review from riodeuno October 26, 2021 12:12

riodeuno reviewed Oct 26, 2021

View reviewed changes

vicky-primathon force-pushed the fix/selected-phone-number-country-code branch from 51c2f21 to 75aad2b Compare October 28, 2021 09:42

vicky-primathon requested a review from riodeuno October 28, 2021 10:58

riodeuno mentioned this pull request Nov 1, 2021

[Task] Move migrations to the backend #8889

Open

5 tasks

vicky-primathon force-pushed the fix/selected-phone-number-country-code branch from 75aad2b to bcb1c96 Compare November 8, 2021 17:25

vicky-primathon added 7 commits November 9, 2021 14:19

Fix countryCode property

dc5c9f4

Show dial_code as countryCode and rename currencyCountryCode to curre…

5fb19b9

…ncyCode

remove unused variable

f11ce70

Add migrations to update selected phone number country code to phone …

f90d5e9

…number dial code

remove console.log

b428d3a

Fix cypress test

e82ee44

Fix: currencyCountryCode to currencyCode binding

a0f2e03

vicky-primathon force-pushed the fix/selected-phone-number-country-code branch 3 times, most recently from 83cd5d1 to c28b203 Compare November 9, 2021 08:57

Remove duplicate currencies from currencyOptions and add dialCode and…

cc7c48b

… code to currency migrations"

vicky-primathon force-pushed the fix/selected-phone-number-country-code branch from c28b203 to cc7c48b Compare November 9, 2021 09:02

vicky-primathon marked this pull request as draft November 9, 2021 11:37

vicky-primathon closed this Nov 12, 2021

mohanarpit deleted the fix/selected-phone-number-country-code branch December 12, 2021 13:57

Conversation

vicky-primathon commented Sep 21, 2021 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Test coverage results 🧪

Uh oh!

riodeuno commented Sep 23, 2021

Uh oh!

somangshu commented Sep 23, 2021

Uh oh!

somangshu left a comment

Choose a reason for hiding this comment

Uh oh!

vicky-primathon commented Sep 23, 2021

Uh oh!

somangshu commented Sep 23, 2021

Uh oh!

somangshu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

riodeuno commented Oct 4, 2021

Uh oh!

vivekverma2312 commented Oct 11, 2021

Uh oh!

vicky-primathon commented Oct 26, 2021

Uh oh!

vicky-primathon commented Oct 26, 2021

Uh oh!

vicky-primathon commented Oct 26, 2021

Uh oh!

vicky-primathon commented Oct 26, 2021

Uh oh!

vicky-primathon commented Oct 26, 2021

Uh oh!

vicky-primathon commented Oct 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riodeuno Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riodeuno commented Oct 26, 2021

Uh oh!

vicky-primathon commented Oct 28, 2021

Uh oh!

vicky-primathon commented Oct 28, 2021

Uh oh!

somangshu commented Nov 9, 2021

Uh oh!

riodeuno commented Nov 9, 2021

Uh oh!

somangshu commented Nov 9, 2021

Uh oh!

sbalaji1192 commented Nov 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

vicky-primathon commented Sep 21, 2021 •

edited by github-actions bot

Loading

riodeuno Oct 27, 2021 •

edited

Loading